Azure.Identity adding msi implementation for AppService and CloudShell by schaabs · Pull Request #6699 · Azure/azure-sdk-for-net

schaabs · 2019-06-25T17:55:28Z

No description provided.

…nto msi-full-impl

daviwil

LGTM

daviwil · 2019-06-25T18:05:07Z

+        private SemaphoreSlim _initLock = new SemaphoreSlim(1, 1);
+
+
+        private readonly Uri ImdsEndptoint = new Uri("http://169.254.169.254/metadata/identity/oauth2/token");


These two constants are probably not needed now that they're in ManagedIdentityClient

chlowell · 2019-06-25T18:29:58Z

+                            // if ONLY the env var MSI_ENDPOINT is set the MsiType is CloudShell
+                            else
+                            {
+                                s_msiType = MsiType.AppService;


MsiType.CloudShell?

chlowell · 2019-06-25T18:31:00Z

+                            // if ONLY the env var MSI_ENDPOINT is set the MsiType is CloudShell
+                            else
+                            {
+                                s_msiType = MsiType.AppService;


MsiType.CloudShell?

chlowell · 2019-06-25T18:43:33Z

+
+                request.UriBuilder.Uri = ImdsEndpoint;
+
+                request.UriBuilder.AppendQuery("api-version", ImdsApiVersion);


It's okay if we get a 400 due to no query string.

I feel like specifying the API version is good. Possibly future versions won't require the Metadata header. I know that this is unlikely but adding the api-version is very low cost.

Good point, I'll add it in the Python library as well.

chlowell · 2019-06-25T18:51:45Z

+
+            request.Method = HttpPipelineMethod.Get;
+
+            request.Headers.Add("secret", s_secret);


Docs claim the secret rotates. I don't know whether that requires an application restart so I get the value from the environment for each request.

good catch. updated

chlowell · 2019-06-25T18:58:50Z

+        private Request CreateAppServiceAuthRequest(string[] scopes, string clientId)
+        {
+            // covert the scopes to a resource string
+            string resource = ScopeUtilities.ScopesToResource(scopes);


I think the managed identity endpoints require one scope per request.

ScopesToResource enforces only one scope is specified. I left all the signatures as a string[] to keep it uniform with the outermost api.

public static string ScopesToResource(string[] scopes) { if (scopes == null) throw new ArgumentNullException(nameof(scopes)); if (scopes.Length != 1) throw new ArgumentException("To convert to a resource string the specified array must be exactly length 1", nameof(scopes));

KrzysztofCwalina

In general, it looks good. The main top level comment I would have is that the code could use more comments :-)

KrzysztofCwalina · 2019-06-25T20:45:34Z

+            {
+                var response = await _pipeline.SendRequestAsync(request, cancellationToken).ConfigureAwait(false);
+
+                if (response.Status == 200 || response.Status == 201)


ClientOptions has a property called ResponseClassifier. Should it be used here to determine if the code is success? I assume you don't want to do it, but then why do we have ResponseClassifier on options?

The behavior of this client and AadIdentityClient differ in what codes are failure, and what codes should be retried. Currently we're using the IdentityClientOptions class for both clients but this needs to be changed. I've added issue #6711 to track this.

KrzysztofCwalina · 2019-06-25T20:47:38Z

+                    return result;
+                }
+
+                throw response.CreateRequestFailedException();


Should we add some custom message here? i.e. is there some action we could recommend users take when this fails? The default message just shows the the response line and headers (not even the body, I think).

We should add an exception type so that these failures can be easily distinguished from service client failures, as both would originate from the same user call to a service client method. I've added issue #6712 to track this.

KrzysztofCwalina · 2019-06-25T21:26:05Z

+                }
+            }
+
+            return new AccessToken(accessToken, expiresOn);


What happens id access_token does not exist in the JSON payload? i.e. is it ok to pass null here?

If the access_token to not exist here then the Token property on AccessToken will be null and this will be handled higher up in the stack. So while it didn't get a token, it should not throw an exception from here.

KrzysztofCwalina · 2019-06-25T21:31:55Z

+        {
+            if (cancellationToken != default)
+            {
+                Task.Delay(1000, cancellationToken).GetAwaiter().GetResult();


@stephentoub, is this reliable? BTW, we are doing it as Thread.Sleep does not have an overload that takes a cancellation token.

It's "reliable" in that it'll sleep for at least a second. It is effectively sync-over-async, though, in that this will block the current thread until the async operation completes, which necessitates a thread pool thread briefly, so if this were production code and this was running on a thread pool thread, I'd be more concerned.

Thread.Sleep doesn't take a cancellation token as it just maps to the OS' thread sleep functionality, e.g. ::Sleep(milliseconds) on Windows, where there's no notion of cancellation. We could of course add another cancelable overload in the future, at which point we'd not just use the OS functionality but isntead do something like the below...

If this were production code, and you wanted to synchronously block the current thread for X milliseconds or until a cancellation request occurred, I'd probably instead advocate for something more like:

private static readonly ManualResetEventSlim s_sleeper = new ManualResetEventSlim(); ... s_sleeper.Wait(1000, cancellationToken);

but for test code, what you have is fine.

johanste · 2019-06-25T22:51:08Z

+    internal class AadClient
    {
-        private static Lazy<IdentityClient> s_sharedClient = new Lazy<IdentityClient>(() => new IdentityClient());
+        private static Lazy<AadClient> s_sharedClient = new Lazy<AadClient>(() => new AadClient());


Are there any scenarios under which s_sharedClient is rendered unusable? E.g. a fatal error that it is not possible to recover from without recreating the client instance?

The client is mostly stateless. The only errors which would render the client useless would be through miss configuration of the process, which would mean the s_sharedClient would never be usable and error on all calls.

johanste · 2019-06-25T23:00:07Z

+            {
+                var response = await _pipeline.SendRequestAsync(request, cancellationToken).ConfigureAwait(false);
+
+                if (response.Status == 200 || response.Status == 201)


Out of curiosity, under what circumstances would we get a 201 response?

This logic needs to be moved into the ClientOptions. I've added issue #6711 to track this.

johanste · 2019-06-25T23:04:33Z

+                        {
+                            s_endpoint = new Uri(endpointEnvVar);
+
+                            // if BOTH the env vars MSI_ENDPOINT and MSI_SECRET are set the MsiType is AppService


What environment variables do aks/aci/acs use?

johanste · 2019-06-25T23:13:06Z

+                // this indicates that the request timed out and that imds is not available.  Otherwise the operation
+                // was user cancelled.  In this case we don't wan't to handle the exception so s_identityAvailable will
+                // remain unset, as we still haven't determined if the imds endpoint is available.
+                catch (OperationCanceledException ex) when (ex.CancellationToken == imdsTimeout)


can you check cancellationToken.IsCancellationRequested?

…rsion

Scott Schaab added 5 commits June 19, 2019 16:00

Renaming classes for cross-language consistency

2eccf6b

Adding msi implementation for AppService and CloudShell

ed5f845

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-net i…

522a4ac

…nto msi-full-impl

using shared FormUrlEncodedContentType header

4c4386f

renaming IdentityClient.cs to match class name AadClient

6ba6170

schaabs requested review from KrzysztofCwalina, chlowell and daviwil June 25, 2019 17:55

daviwil approved these changes Jun 25, 2019

View reviewed changes

removing unneeded fields from ManagedIdentityCredential

ec53033

chlowell reviewed Jun 25, 2019

View reviewed changes

updates addressing PR feedback

d203819

schaabs requested a review from chlowell June 25, 2019 20:44

fixing compiler error introduced with PR updates

71c06ec

KrzysztofCwalina reviewed Jun 25, 2019

View reviewed changes

more updates addressing pr feedback

912e89d

johanste approved these changes Jun 25, 2019

View reviewed changes

Scott Schaab added 4 commits June 25, 2019 17:45

fix for imds availability timeout check

62d8327

fix AuthenticateAsync to call GetMsiTypeAsync rather than the sync ve…

a9a7f5e

…rsion

rename AadClient -> AadIdentityClient

16895eb

adding missed renamed file

4964552

schaabs merged commit 559a33d into Azure:master Jun 26, 2019

schaabs deleted the msi-full-impl branch June 26, 2019 17:51

		private SemaphoreSlim _initLock = new SemaphoreSlim(1, 1);


		private readonly Uri ImdsEndptoint = new Uri("http://169.254.169.254/metadata/identity/oauth2/token");


		request.UriBuilder.Uri = ImdsEndpoint;

		request.UriBuilder.AppendQuery("api-version", ImdsApiVersion);


		request.Method = HttpPipelineMethod.Get;

		request.Headers.Add("secret", s_secret);

Conversation

schaabs commented Jun 25, 2019

Uh oh!

daviwil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KrzysztofCwalina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

stephentoub Jun 26, 2019 •

edited

Loading